Add RBAC for virtual attributes in API #15145

lpichler · 2017-05-18T11:54:58Z

So called virtual attributes are passed to API thru ‘attributes’ parameter and such attributes can be:

direct association
e.g. attributes=vms (plural associations - collection)
e.g. attributes=parent_manage (belongs_to association)

/api/providers/2?attributes=parent_manager,vms

indirect association
e.g. attributes=parent_manager.cloud_tenants (plural associations - collection)
e.g. attributes=parent_manager.host (belongs_to association)

/api/providers/2?attributes=parent_manager.host, parent_manager.cloud_tenants

This RBAC situation is covered by method virtual_attribute_search(resource, attribute)
where the attribute can be has_many association or belongs_to (There are different RBAC check)

it can also be for the pure class as MiqRequestWorkflow (last if-else leg)

Links

Gaprindashvili BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1448857
FINE BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1452868

PR for EUWE: #15143 there were CloudTenants listed in controller, there it is listed by API

lpichler · 2017-05-19T12:33:55Z

@miq-bot add_label fine/yes, bug, api, rbac
cc @isimluk @imtayadeway @yrudman
@miq-bot assign @gtanzillo

lpichler · 2017-05-19T12:36:42Z

app/controllers/api/base_controller/renderer.rb

@@ -205,7 +217,7 @@ def expand_virtual_attributes(json, type, resource)
      def fetch_direct_virtual_attribute(type, resource, attr)
        return unless attr_accessible?(resource, attr)
        virtattr_accessor = virtual_attribute_accessor(type, attr)


Not sure how virtattr_accessor ( send(virtattr_accessor, resource) is used.
Should it be also in RBAC ? What are examples of using it ?

used by API when it needs extra logic for fetching an attribute, i.e. look at any of the fetch__attr named methods. currently implemented for service_dialogs content and orchestration_stacks stdout, so not traversing relationships and we're already rbac'd on the resource itself, so we're ok with those.

gtanzillo · 2017-05-23T13:48:09Z

@abellotti Please review when you have some time.

gtanzillo · 2017-05-23T13:54:31Z

app/controllers/api/base_controller/renderer.rb

+          if resource.class.plural?(attribute)
+            Rbac.filtered(resource.public_send(attribute))
+          else
+            Rbac.filtered_object(resource).public_send(attribute)


Does this handle the case where the resource is filtered out due to RBAC?

👍 I added try when is the resource is filtered out.

lpichler · 2017-05-26T10:35:54Z

FINE BZ:https://bugzilla.redhat.com/show_bug.cgi?id=1452868

gtanzillo

LGTM 👍

abellotti · 2017-05-30T14:20:18Z

app/controllers/api/base_controller/renderer.rb

+          if resource.class.try(:plural?, attribute)
+            Rbac.filtered(resource.public_send(attribute))
+          else
+            Rbac.filtered_object(resource).try(:public_send, attribute)


is the Rbac.filtered_object(resource) on line 173 needed ? We already did the RBAC on the resource, isn't resource.public_send(attribute) sufficient ? Thanks.

there are no RBAC check where associations are requested thru attributes params like /api/providers/2?attributes=parent_manager.host, parent_manager.cloud_tenants

we have RBAC check for collections and as you said for resource but not for the ?attributes

kbrock · 2017-05-30T22:15:00Z

@lpichler I'm leaning towards reflection_plural?

The goal of ar_virtual was to go away after virtual reflections and virtual columns were merged into active record base. Or something like that.

Is there another place to put this code other than ar_virtual.rb?

Virtual attributes are passed to API thru ‘attributes’ parameter and it can be: - direct association e.g. attributes=vms (plural associations - collection) e.g. attributes=parent_manage (belongs_to association) /api/providers/2?attributes=parent_manages,vms - indirect association e.g. attributes=parent_manager.cloud_tenants (plural associations - collection) e.g. attributes=parent_manager.host (belongs_to association) localhost:3000/api/providers/2?attributes=parent_manager.host, parent_manager.cloud_tenants This RBAC situation is covered by method virtual_attribute_search(resource, attribute) where attribute can be has_many association or belongs_to (There are different RBAC check) it can also pure class as MiqRequestWorkflow (last if-else leg)

lpichler · 2017-06-07T15:10:35Z

@miq-bot remove_label wip

miq-bot · 2017-06-07T15:12:48Z

Checked commits lpichler/manageiq@75f0552~...0d479b1 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

lpichler · 2017-06-07T15:15:03Z

Thanks for the review.

Is there another place to put this code other than ar_virtual.rb?

@kbrock I put it in the method here with the comment and removed it from ar_virtual.rb, I didn't find the better place.

@gtanzillo @abellotti
PR should be ready.

Add RBAC for virtual attributes in API (cherry picked from commit bec2d7a) https://bugzilla.redhat.com/show_bug.cgi?id=1452868

simaishi · 2017-06-14T15:21:57Z

Fine backport details:

$ git log -1
commit 0f10ac6d15da1e958e4bbc10d82cc3ca4e7a0a6f
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Wed Jun 14 09:43:51 2017 -0400

    Merge pull request #15145 from lpichler/add_rbac_for_virtual_attributes
    
    Add RBAC for virtual attributes in API
    (cherry picked from commit bec2d7ad99f86a2bac750d649c5e80bfcd0efbf3)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1452868

miq-bot added the wip label May 18, 2017

lpichler force-pushed the add_rbac_for_virtual_attributes branch 5 times, most recently from 50f0aa3 to 5aaa3c8 Compare May 19, 2017 12:27

lpichler changed the title ~~[WIP] Add RBAC for virtual attributes in API~~ Add RBAC for virtual attributes in API May 19, 2017

miq-bot removed the wip label May 19, 2017

miq-bot added api bug fine/yes core/rbac labels May 19, 2017

miq-bot assigned gtanzillo May 19, 2017

lpichler commented May 19, 2017

View reviewed changes

lpichler mentioned this pull request May 19, 2017

"Add New Cloud Volume" dialog does not restrict cloud tenants for non-admins users. #14836

Closed

gtanzillo requested a review from abellotti May 22, 2017 22:05

lpichler mentioned this pull request May 23, 2017

[EUWE] Add RBAC check for CloudTenant in CloudVolume form #15143

Merged

gtanzillo reviewed May 23, 2017

View reviewed changes

lpichler force-pushed the add_rbac_for_virtual_attributes branch from 5aaa3c8 to 8c90800 Compare May 29, 2017 16:15

lpichler closed this May 30, 2017

lpichler reopened this May 30, 2017

lpichler force-pushed the add_rbac_for_virtual_attributes branch 2 times, most recently from b2b834e to 3940718 Compare May 30, 2017 13:22

gtanzillo approved these changes May 30, 2017

View reviewed changes

abellotti reviewed May 30, 2017

View reviewed changes

lpichler changed the title ~~Add RBAC for virtual attributes in API~~ [WIP] Add RBAC for virtual attributes in API May 31, 2017

miq-bot added the wip label May 31, 2017

lpichler force-pushed the add_rbac_for_virtual_attributes branch from 3940718 to 77d317a Compare June 7, 2017 14:52

lpichler added 2 commits June 7, 2017 17:10

Add specs for API RBAC tests for direct and indirect associations

0d479b1

lpichler force-pushed the add_rbac_for_virtual_attributes branch from 77d317a to 0d479b1 Compare June 7, 2017 15:10

lpichler changed the title ~~[WIP] Add RBAC for virtual attributes in API~~ Add RBAC for virtual attributes in API Jun 7, 2017

miq-bot removed the wip label Jun 7, 2017

gtanzillo added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 14, 2017

gtanzillo merged commit bec2d7a into ManageIQ:master Jun 14, 2017

lpichler deleted the add_rbac_for_virtual_attributes branch June 14, 2017 13:48

simaishi pushed a commit that referenced this pull request Jun 14, 2017

Merge pull request #15145 from lpichler/add_rbac_for_virtual_attributes

0f10ac6

Add RBAC for virtual attributes in API (cherry picked from commit bec2d7a) https://bugzilla.redhat.com/show_bug.cgi?id=1452868

simaishi added fine/backported and removed fine/yes labels Jun 14, 2017

NickLaMuro mentioned this pull request Feb 7, 2019

Performance updates for virtual_attribute_search ManageIQ/manageiq-api#557

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RBAC for virtual attributes in API #15145

Add RBAC for virtual attributes in API #15145

lpichler commented May 18, 2017 •

edited

Loading

lpichler commented May 19, 2017

lpichler May 19, 2017

abellotti May 30, 2017

gtanzillo commented May 23, 2017

gtanzillo May 23, 2017

lpichler May 30, 2017

lpichler commented May 26, 2017

gtanzillo left a comment

abellotti May 30, 2017

lpichler May 30, 2017

kbrock commented May 30, 2017

lpichler commented Jun 7, 2017

miq-bot commented Jun 7, 2017

lpichler commented Jun 7, 2017

simaishi commented Jun 14, 2017

Add RBAC for virtual attributes in API #15145

Add RBAC for virtual attributes in API #15145

Conversation

lpichler commented May 18, 2017 • edited Loading

Links

lpichler commented May 19, 2017

lpichler May 19, 2017

Choose a reason for hiding this comment

abellotti May 30, 2017

Choose a reason for hiding this comment

gtanzillo commented May 23, 2017

gtanzillo May 23, 2017

Choose a reason for hiding this comment

lpichler May 30, 2017

Choose a reason for hiding this comment

lpichler commented May 26, 2017

gtanzillo left a comment

Choose a reason for hiding this comment

abellotti May 30, 2017

Choose a reason for hiding this comment

lpichler May 30, 2017

Choose a reason for hiding this comment

kbrock commented May 30, 2017

lpichler commented Jun 7, 2017

miq-bot commented Jun 7, 2017

lpichler commented Jun 7, 2017

simaishi commented Jun 14, 2017

lpichler commented May 18, 2017 •

edited

Loading